check-by-name: Refactor to prepare for enforcing pkgs/by-name, make --base required
#278805
Conversation
Does a bunch of cleanups to the eval.{rs,nix} code to make future
changes easier, no functionality is changed.
This was previously a checking impurity that could produce different results when run on different systems.
CI now passes the flag, so it doesn't have to be optional anymore
infinisil
changed the title
pkgs/by-name, make --base requiredpkgs/by-name, make --base required
This makes the attribute ratchet check logic more re-usable, which will be used in a future commit. It also renames the ratchet states to something more intuitive
Makes errors for attributes deterministic so it's easier to test (also, reproducibility is always nice)
Strips the Nixpkgs prefix from the callPackage paths, makes future error messages using this path be deterministic.
infinisil
force-pushed
the
by-name-enforce-preparation
branch
from
07eee89 to
54b0532
Compare
There was a problem hiding this comment.
I'm assuming this is a sentinel file.
There was a problem hiding this comment.
Yeah, just so that git can track that the directory exists. Marginally easier than changing the code to handle a missing directory
There was a problem hiding this comment.
OK, I took a look at the Rust code here, and skimmed the test-only Nix code.
I have three non-blocking refactoring suggestions, in descending order of "want to see":
- Simplify the traits in
ratchet.rsand use them more broadly. - Use
anyhow::Context::with_contextto avoid allocating error strings in the non-error case. - Minor typo and comment fixes as noted.
There's a slightly larger refactoring that I see that's not directly related to the changes in this PR. In main, everywhere the error_writer is used could instead use a ratchet::Nixpkgs that's cased on its trivial success.
| /// What error to produce in case the ratchet went in the wrong direction | ||
| fn to_error(name: &str, context: &Self, existed_before: bool) -> NixpkgsProblem; |
There was a problem hiding this comment.
This looks like a IntoNixpkgsProblem trait. I think it can consume Self, and have context be self.
There was a problem hiding this comment.
It looks like making it consume Self is a bit trickier, since the values come from a HashMap originally. And not sure what you mean with context being self!
| Missing => NixpkgsProblem::UndefinedAttr { | ||
| relative_package_file: relative_package_file.clone(), | ||
| package_name: attribute_name.clone(), | ||
| } | ||
| .into(), |
There was a problem hiding this comment.
This looks like it's implementing IntoNixpkgsProblem on ByNameAttribute.
The same thing holds for the cases below on their specific types.
There was a problem hiding this comment.
Not entirely because in some cases there won't be an error.
There was a problem hiding this comment.
Not entirely because in some cases there won't be an error.
Agreed. I took a swing at this (PR not sent out) and ran into the same thing. There's some fine tuning that could be done for sure but I think the trait-based approach I suggested isn't right.
delroth
added
the
12.approvals: 1
delroth
added
12.approvals: 2
- Typo - Rename AttributeRatchet to ToNixpkgsProblem - Make the compare trait method into a RatchetState method Co-Authored-By: Philip Taron <[email protected]>
Avoids allocation in the non-error case
|
Feedback addressed, I'll merge once CI passes since the feedback isn't blocking :) |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-01-09-nixpkgs-architecture-team-meeting-47/38037/1 |
delroth
removed
the
12.approvals: 2
| Missing => NixpkgsProblem::UndefinedAttr { | ||
| relative_package_file: relative_package_file.clone(), | ||
| package_name: attribute_name.clone(), | ||
| } | ||
| .into(), |
There was a problem hiding this comment.
Not entirely because in some cases there won't be an error.
Agreed. I took a swing at this (PR not sent out) and ran into the same thing. There's some fine tuning that could be done for sure but I think the trait-based approach I suggested isn't right.
Description of changes
This is a preparation PR for #275539, including only internal improvements to the code and comments, without changing any functionality. This makes the diff for the above PR much smaller.
The only somewhat interesting change is that
--baseis now required fornixpkgs-check-by-name, which was already prepared for in #274591 by always passing the flag in CI.This PR is best reviewed commit-by-commit.
Things done
x86_64-linuxAdd a 👍 reaction to pull requests you find important.